chromium_ec: Add host command to read board ID #236
Merged
+116
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
|
Need to add a command to read them and a note that it's not supported on all platforms. |
JohnAZoidberg
force-pushed
the
boardid
branch
from
510b712 to
8634eee
Compare
JohnAZoidberg
force-pushed
the
boardid
branch
2 times, most recently
from
878250a to
f5cf9a1
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds functionality to read and display board IDs for various hardware components via a new host command to the Chromium EC. The implementation introduces a new --boardid command-line flag that queries the EC for board IDs of different components like the mainboard, power button board, touchpad, audio board, and dGPU modules.
Changes:
- Added EC host command (
ReadBoardId) to query board IDs from the embedded controller - Implemented command-line flag
--boardidto trigger board ID reading - Created board ID type enumeration for different hardware components
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| framework_lib/src/chromium_ec/command.rs | Added new EC command enum value for ReadBoardId |
| framework_lib/src/chromium_ec/commands.rs | Defined BoardIdType enum and request/response structures for board ID reading |
| framework_lib/src/chromium_ec/mod.rs | Implemented read_board_id_hc method to handle board ID queries |
| framework_lib/src/commandline/mod.rs | Added boardid CLI field and print_board_ids function to display results |
| framework_lib/src/commandline/clap_std.rs | Added boardid argument to clap CLI parser |
| framework_lib/src/commandline/uefi.rs | Added boardid argument to UEFI CLI parser |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1743
to
+1754
| pub fn read_board_id_hc(&self, board_id_type: BoardIdType) -> EcResult<Option<u8>> { | ||
| let res = EcRequestReadBoardId { | ||
| board_id_type: board_id_type as u8, | ||
| } | ||
| .send_command(self)?; | ||
| match res.board_id { | ||
| -1 => Err(EcError::DeviceError(format!( | ||
| "Failed to read Board ID {:?}", | ||
| board_id_type | ||
| ))), | ||
| 15 => Ok(None), | ||
| 0..=14 => Ok(Some(res.board_id as u8)), |
There was a problem hiding this comment.
The magic numbers -1 and 15 should be defined as named constants to improve code clarity. Consider defining constants like BOARD_ID_INVALID = -1 and BOARD_ID_NOT_PRESENT = 15 near the function or in the BoardIdType enum implementation.
Suggested change
| pub fn read_board_id_hc(&self, board_id_type: BoardIdType) -> EcResult<Option<u8>> { | |
| let res = EcRequestReadBoardId { | |
| board_id_type: board_id_type as u8, | |
| } | |
| .send_command(self)?; | |
| match res.board_id { | |
| -1 => Err(EcError::DeviceError(format!( | |
| "Failed to read Board ID {:?}", | |
| board_id_type | |
| ))), | |
| 15 => Ok(None), | |
| 0..=14 => Ok(Some(res.board_id as u8)), | |
| const BOARD_ID_INVALID: i32 = -1; | |
| const BOARD_ID_NOT_PRESENT: i32 = 15; | |
| const BOARD_ID_MAX_VALID: i32 = BOARD_ID_NOT_PRESENT - 1; | |
| pub fn read_board_id_hc(&self, board_id_type: BoardIdType) -> EcResult<Option<u8>> { | |
| let res = EcRequestReadBoardId { | |
| board_id_type: board_id_type as u8, | |
| } | |
| .send_command(self)?; | |
| match res.board_id { | |
| BOARD_ID_INVALID => Err(EcError::DeviceError(format!( | |
| "Failed to read Board ID {:?}", | |
| board_id_type | |
| ))), | |
| BOARD_ID_NOT_PRESENT => Ok(None), | |
| 0..=BOARD_ID_MAX_VALID => Ok(Some(res.board_id as u8)), |
Member
Author
There was a problem hiding this comment.
Hm let's not do that for now
| #[repr(C, packed)] | ||
| #[derive(Clone, Copy, PartialEq, Eq, Debug)] | ||
| pub struct EcResponseReadBoardId { | ||
| /// Board ID (-1 invalid, 15 not present) |
There was a problem hiding this comment.
The comment describes the special values but doesn't explain what values 0-14 represent. Add documentation explaining that valid board IDs are in the range 0-14.
Suggested change
| /// Board ID (-1 invalid, 15 not present) | |
| /// Board ID: 0–14 valid, -1 invalid, 15 not present |
This command is not present on all systems, it was only recently added to our main branch and will be slowly backported. Signed-off-by: Daniel Schaefer <[email protected]>
JohnAZoidberg
force-pushed
the
boardid
branch
from
f5cf9a1 to
bf2ee98
Compare
Using a native host command to get the ID from the EC instead of having the sotware measure ADC and evaluate the host command. This is better because the ADC values represent different board IDs on different platforms (Microchip vs Nuvoton based ECs) > sudo framework_tool --boardid Board IDs Mainboard: Ok(Some(6)) PowerButton: Err(Response(InvalidResponse)) Touchpad: Ok(Some(7)) AudioBoard: Ok(Some(7)) dGPU0: Err(Response(InvalidCommand)) dGPU1: Err(Response(InvalidCommand)) ``` Signed-off-by: Daniel Schaefer <[email protected]>
JohnAZoidberg
force-pushed
the
boardid
branch
from
bf2ee98 to
4915562
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.